Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add lease collector #1038

Merged
merged 1 commit into from
Jan 30, 2020
Merged

Add lease collector #1038

merged 1 commit into from
Jan 30, 2020

Conversation

qw1mb0
Copy link
Contributor

@qw1mb0 qw1mb0 commented Jan 26, 2020 •
edited
Loading

What this PR does / why we need it:
Does kubelet have 2 mechanisms for confirming that node is available: https://kubernetes.io/docs/concepts/architecture/nodes/#heartbeats

Unfortunately, nodeStatus is updated every 5 minutes, which is too much, but in Lease object this information is updated every 10 seconds. Having information about the updateTime kubelet in the metrics, we will be able to determine much faster that something has happened to the node.

This collector adds a few new metrics for each Lease object.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
No issue about lease objects.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 26, 2020
brancz
brancz reviewed Jan 27, 2020
View reviewed changes
docs/lease-metrics.md Outdated Show resolved Hide resolved
brancz
brancz reviewed Jan 27, 2020
View reviewed changes
docs/lease-metrics.md Outdated Show resolved Hide resolved
@qw1mb0
Copy link
Contributor Author

qw1mb0 commented Jan 28, 2020

How to fix the problem correctly https://travis-ci.org/kubernetes/kube-state-metrics/jobs/642760515?utm_medium=notification&utm_source=github_status ?

@brancz
Copy link
Member

brancz commented Jan 28, 2020

You need jsonnet and jb installed and then run “make validate-manifests”.

brancz
brancz reviewed Jan 28, 2020
View reviewed changes
internal/store/lease.go Outdated Show resolved Hide resolved
brancz
brancz reviewed Jan 28, 2020
View reviewed changes
internal/store/lease.go Outdated Show resolved Hide resolved
@brancz
Copy link
Member

brancz commented Jan 28, 2020

Looks like there is a CI failure regarding linting. You can run make lint to reproduce this in your environment.

@qw1mb0
Copy link
Contributor Author

qw1mb0 commented Jan 28, 2020 •
edited
Loading

Looks like there is a CI failure regarding linting. You can run make lint to reproduce this in your environment.

Fixed in this commit: b82adc4

@brancz
Copy link
Member

brancz commented Jan 28, 2020

Could you squash your changes? Otherwise this lgtm

tariq1890
tariq1890 reviewed Jan 28, 2020
View reviewed changes
internal/store/lease.go Outdated
@@ -0,0 +1,113 @@
/*
Copyright 2016 The Kubernetes Authors All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2020

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

internal/store/lease_test.go Show resolved Hide resolved
tests/manifests/lease.yaml Outdated
spec:
holderIdentity: kube-master
leaseDurationSeconds: 40
renewTime: "2020-01-26T09:52:23.548762Z"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need EOL here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

internal/store/builder.go Outdated
@@ -187,6 +188,7 @@ var availableStores = map[string]func(f *Builder) cache.Store{
"validatingwebhookconfigurations": func(b *Builder) cache.Store { return b.buildValidatingWebhookConfigurationStore() },
"volumeattachments": func(b *Builder) cache.Store { return b.buildVolumeAttachmentStore() },
"verticalpodautoscalers": func(b *Builder) cache.Store { return b.buildVPAStore() },
"leases": func(b *Builder) cache.Store { return b.buildLeases() },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lexical order please

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

docs/README.md Outdated
@@ -73,6 +73,7 @@ Per group of metrics there is one file for each metrics. See each file for speci
- [ValidatingWebhookConfiguration Metrics](validatingwebhookconfiguration.md)
- [VerticalPodAutoscaler Metrics](verticalpodautoscaler-metrics.md)
- [VolumeAttachment Metrics](volumeattachment-metrics.md)
- [Lease Metrics](lease-metrics.md)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lexical order

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@qw1mb0
Copy link
Contributor Author

qw1mb0 commented Jan 29, 2020

Could you squash your changes? Otherwise this lgtm

done

@brancz
Copy link
Member

brancz commented Jan 29, 2020

lgtm but giving @tariq1890 another chance to review

tariq1890
tariq1890 approved these changes Jan 29, 2020
View reviewed changes
internal/store/lease_test.go Outdated
c.Func = generator.ComposeMetricGenFuncs(leaseMetricFamilies)
c.Headers = generator.ExtractMetricFamilyHeaders(leaseMetricFamilies)
if err := c.run(); err != nil {
t.Errorf("unexpected collecting result in %vth run:\n%s", i, err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
t.Errorf("unexpected collecting result in %vth run:\n%s", i, err)
t.Errorf("unexpected collecting result in %dth run:\n%v", i, err)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

pkg/options/collector.go Outdated
@@ -53,5 +53,6 @@ var (
"storageclasses": struct{}{},
"validatingwebhookconfigurations": struct{}{},
"volumeattachments": struct{}{},
"leases": struct{}{},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be in lexical order too please.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

docs/lease-metrics.md Outdated
@@ -0,0 +1,6 @@
# Leases Metrics
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Leases Metrics
# Lease Metrics

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

docs/lease-metrics.md
| Metric name| Metric type | Labels/tags | Status |
| ---------- | ----------- | ----------- | ----------- |
| kube_lease_renew_time | Gauge | `lease`=&lt;lease-name&gt; | EXPERIMENTAL |
| kube_lease_owner | Gauge | `lease`=&lt;lease-name&gt; <br> `owner_kind`=&lt;onwer kind&gt; <br> `owner_name`=&lt;owner name&gt; | EXPERIMENTAL |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be in lexical order too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@tariq1890
Copy link
Contributor

Sorry, the PR approve was by mistake. Almost there :). Thanks for your hard work!

@tariq1890
Copy link
Contributor

Once these items are addressed, the PR should be good to be merge. FYI @brancz @lilic

@brancz
Copy link
Member

brancz commented Jan 30, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 30, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brancz, qw1mb0

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 30, 2020
@k8s-ci-robot k8s-ci-robot merged commit 49672c7 into kubernetes:master Jan 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brancz brancz brancz left review comments

@tariq1890 tariq1890 tariq1890 approved these changes

@andyxning andyxning Awaiting requested review from andyxning

@zouyee zouyee Awaiting requested review from zouyee

Assignees

@brancz brancz

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@qw1mb0 @brancz @tariq1890 @k8s-ci-robot